Skip to content

feat(evo-marko): evo-combobox#661

Open
LuLaValva wants to merge 6 commits into
mainfrom
evo-combobox
Open

feat(evo-marko): evo-combobox#661
LuLaValva wants to merge 6 commits into
mainfrom
evo-combobox

Conversation

@LuLaValva
Copy link
Copy Markdown
Member

Description

  • Add <evo-combobox> to @evo-web/marko

Screenshots

image

Copilot AI review requested due to automatic review settings May 7, 2026 18:36
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 7, 2026

🦋 Changeset detected

Latest commit: 3526052

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@evo-web/marko Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a new evo-combobox tag to @evo-web/marko, including styling, Storybook docs/examples, and SSR/browser tests.

Changes:

  • Introduces evo-combobox Marko implementation, styles, README, and Storybook stories/examples (including async filtering via JSON data).
  • Adds SSR snapshot coverage and browser interaction tests for core combobox behaviors.
  • Updates TypeScript config to support JSON module imports and includes JSON in compilation scope.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
packages/evo-marko/tsconfig.json Enables JSON module support for Storybook/examples and includes JSON files in TS scope.
packages/evo-marko/src/tags/evo-combobox/index.marko Implements the combobox UI, filtering, keyboard/mouse interactions, and markup/ARIA.
packages/evo-marko/src/tags/evo-combobox/style.ts Adds Skin CSS imports required by the new combobox and related UI elements.
packages/evo-marko/src/tags/evo-combobox/combobox.stories.ts Adds Storybook stories and controls for the new component.
packages/evo-marko/src/tags/evo-combobox/README.md Documents usage, props, and attribute tags for evo-combobox.
packages/evo-marko/src/tags/evo-combobox/examples/*.marko Adds examples (default, controllable, async filtering) used by Storybook.
packages/evo-marko/src/tags/evo-combobox/examples/data.json Adds example dataset used by async filtering example.
packages/evo-marko/src/tags/evo-combobox/test/test.server.ts Adds SSR snapshot tests for multiple variants.
packages/evo-marko/src/tags/evo-combobox/test/test.browser.ts Adds browser interaction tests (focus, expand/collapse, keyboard, click, filtering).
packages/evo-marko/src/tags/evo-combobox/test/snapshots/test.server.ts.snap Adds SSR snapshots.
CLAUDE.md Adds Marko 6 guidance notes for extractor and event handler typing patterns.
.changeset/bright-socks-care.md Publishes a patch changeset for @evo-web/marko.

Comment on lines +150 to +157
} else if (e.key === "Enter") {
if (open && activeIndex >= 0) {
e.preventDefault();
value = displayValue ?? visibleOptions[activeIndex].text;
displayValue = null;
}
open = false;
} else if (e.key === "Escape") {
Comment thread packages/evo-marko/src/tags/evo-combobox/index.marko Outdated
Comment thread packages/evo-marko/src/tags/evo-combobox/index.marko Outdated
Comment thread packages/evo-marko/src/tags/evo-combobox/index.marko
Comment thread packages/evo-marko/src/tags/evo-combobox/index.marko
Comment thread packages/evo-marko/src/tags/evo-combobox/index.marko Outdated
Comment thread packages/evo-marko/src/tags/evo-combobox/index.marko Outdated
Comment thread packages/evo-marko/tsconfig.json
Comment thread packages/evo-marko/src/tags/evo-combobox/index.marko Outdated
Comment thread packages/evo-marko/src/tags/evo-combobox/index.marko Outdated
Comment thread packages/evo-marko/src/tags/evo-combobox/index.marko
Comment thread packages/evo-marko/src/tags/evo-combobox/index.marko Outdated
Comment thread packages/evo-marko/src/tags/evo-combobox/index.marko
Comment thread packages/evo-marko/src/tags/evo-combobox/README.md Outdated
Comment thread packages/evo-marko/src/tags/evo-combobox/style.ts Outdated
Copy link
Copy Markdown
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude PR Review. Full list here: https://github.com/eBay/evo-web/tasks/fe6604f5-9c3b-4888-9e89-e3482af049e7

Three blocking issues that will cause test suite failures and a WCAG violation: both test files import stories that don't exist in the stories file (Filtering, FloatingLabel), and non-selected listbox options are missing the required aria-selected attribute. One non-blocking type annotation nit.

Comment thread packages/evo-marko/src/tags/evo-combobox/test/test.browser.ts
Comment thread packages/evo-marko/src/tags/evo-combobox/test/test.server.ts
Comment thread packages/evo-marko/src/tags/evo-combobox/combobox.stories.ts
Comment thread packages/evo-marko/src/tags/evo-combobox/index.marko Outdated
Comment thread packages/evo-marko/src/tags/evo-combobox/index.marko Outdated
Copy link
Copy Markdown
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous blocking issues are all resolved — great cleanup. One new blocking bug introduced by the tempValue refactor: Escape key doesn't clear tempValue, so keyboard-navigated values commit on blur. Three non-blocking observations: redundant aria-owns, always-in-DOM listbox relying on skin CSS, and lost SSR coverage.

}
open = false;
} else if (e.key === "Escape") {
open = false;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: Escape closes the listbox but doesn't clear tempValue. Repro: arrow-navigate to an option (sets tempValue), press Escape (intending to cancel), then click away — onBlur sees tempValue is still set and commits it as the new value, overriding the user's intent to cancel.

Add tempValue = null here:

} else if (e.key === "Escape") {
    open = false;
    activeIndex = -1;
    tempValue = null;  // discard keyboard navigation on cancel
}

value:=value
placeholder=showPlaceholder && placeholder
autocomplete="off"
aria-owns=listboxId
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: aria-owns alongside aria-controls referencing the same element is redundant. The ARIA 1.2 combobox pattern specifies aria-controls to reference the popup listbox — aria-owns is only needed when the DOM parent-child relationship doesn't match the desired accessibility tree structure. Since the listbox is already a descendant of the same combobox widget, aria-controls alone is sufficient. Consider removing aria-owns.

}/>
</if>
</span>
<div
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: The listbox is now always in the DOM (the previous <if=open> guard was removed). AT visibility when the combobox is closed now depends entirely on the skin CSS using display: none for .combobox__listbox when .combobox--expanded is absent — display: none correctly removes the element from the accessibility tree; visibility: hidden or opacity: 0 would not. Worth confirming the skin CSS uses display for this toggle.

});

it("renders with fixed strategy", async () => {
await snapshotHTML(Default, { strategy: "fixed" });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: The previous revision had SSR snapshots for floatingLabel and Filtering (autocomplete='list') modes — they were removed here rather than replaced. The snapshot count is now 6 cases vs. the prior 8. Consider adding:

it("renders floating label", async () => {
    await snapshotHTML(Default, { floatingLabel: "Campaign" });
});
it("renders filtering variant", async () => {
    await snapshotHTML(Default, { autocomplete: "list" });
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants